Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: error docker inspect logs during chalk exec #248

Merged
merged 7 commits into from
Mar 25, 2024
Merged

fix: error docker inspect logs during chalk exec #248

merged 7 commits into from
Mar 25, 2024

Conversation

nettrino
Copy link
Contributor

@nettrino nettrino commented Mar 20, 2024 •
edited by miki725
Loading

Issue

Fixes #247

Description

  • disables docker codec if docker is not installed which removes the misleading error log on chalk exec
  • explicitly falling back to docker command for better stderr when docker is not installed during chalk docker ...

@nettrino nettrino requested a review from viega as a code owner March 20, 2024 08:36
miki725
miki725 previously requested changes Mar 20, 2024
View reviewed changes
Copy link
Contributor

@miki725 miki725 left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The codec is used in 2 modes - dealing with artifacts and docker sub command.
For docker sub command docker inspect error I think should be higher level like it is now however for the general codec handling yeah it should be hidden. In our infra even with this trace level it will still show up as error log since the "error:" comes from the sub command call. We can fix it better I think to completely avoid any error logs during docker exec entrypoint.

@nettrino
Copy link
Contributor Author

@miki725 you want me to make changes in the current PR or close it and it will be addressed once the exec errors are handled properly?

@miki725
Copy link
Contributor

miki725 commented Mar 20, 2024

Let's leave open for now and we can push improvements to this branch

@miki725
Copy link
Contributor

miki725 commented Mar 21, 2024

pushed some changes. note that found a bug in nimutils so full output will be replicated once thats bumped here crashappsec/nimutils#65

➜ docker run -it --rm -v ./chalk:/chalk alpine /chalk --trace docker build -f Dockerfile . 2>&1 | grep -E '(docker|error)'
trace: Searching PATH for 'docker'
trace: Searching path for docker
trace: Could not find 'docker' in path.
warn:  No docker command found in PATH. `chalk docker` not available.
warn:  Disabling docker codec as docker command is not available
error: docker command is missing. chalk requires docker binary installed to wrap docker commands.
docker: No such file or directory

➜ docker run -it --rm -v ./chalk:/chalk alpine /chalk --trace exec -- sleep 1 2>&1 | grep -E '(docker|error)'
trace: Searching PATH for 'docker'
trace: Searching path for docker
trace: Could not find 'docker' in path.
warn:  No docker command found in PATH. `chalk docker` not available.
warn:  Disabling docker codec as docker command is not available

@miki725 miki725 dismissed their stale review March 21, 2024 03:56

changed error handling

@miki725
Copy link
Contributor

miki725 commented Mar 21, 2024

looks like disabling the plugin that way breaks everything. will have to debug whats going on and how to fix all the tests

nettrino and others added 3 commits March 22, 2024 09:55
@nettrino @miki725
nit:log docker inspect errors at trace level
0f86ea1
@miki725
refactor: docker docker is not enabled when docker is not installed
1916318 
otherwise the codec would attempt to use docker commands to interact
with the artifacts which would result in an error which would show
misleading error logs

now if docker is missing, codec is disabled

in addition to optimize the docker wrapping, when docker is missing
it immediately goes to fallback mode which then bubbles up appropriate
error to the user
@miki725
refactor: reverting docker inspect failure log level
33740bf
@miki725 miki725 force-pushed the nettrino/errornit branch from 485462a to 2982e28 Compare March 22, 2024 13:55
@miki725 miki725 requested a review from ee7 March 22, 2024 13:56
ee7
ee7 previously approved these changes Mar 22, 2024
View reviewed changes
Copy link
Contributor

@ee7 ee7 left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests are failing with:

Error: unhandled exception: key not found: docker [KeyError] [inspect]

Otherwise looks good, though I'd suggest we:

  • Clarify the changelog, similar to that suggested.
  • Avoid Nit: for the merged commit title. If we consider this a user-facing improvement to misleading behavior, our convention requires the prefix to be fix:.

(Edit: meant to "comment", not "approve").

CHANGELOG.md Outdated Show resolved Hide resolved
@ee7 ee7 self-requested a review March 22, 2024 14:39
@miki725 miki725 changed the title nit:log docker inspect errors at trace level fix: error docker inspect logs during chalk exec Mar 22, 2024
miki725 added 2 commits March 22, 2024 11:03
@miki725
fix: cant use getDockerExePath in codec init
3683886
@miki725
refactor: add plugin.enabled flag
c029f43 
There are places where plugins are loaded explicitly by name
and so conditionally registering codecs leads to KeyErrors.
By adding enabled flag we can gracefully ignore some callbacks
from plugin definitions without breaking anything relying on plugin
existence.
@miki725 miki725 force-pushed the nettrino/errornit branch from 2982e28 to c029f43 Compare March 22, 2024 15:03
ee7
ee7 previously approved these changes Mar 22, 2024
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
tests/Dockerfile
COPY --from=gcr.io/projectsigstore/cosign /ko-app/cosign /usr/local/bin/cosign
COPY --from=gcr.io/projectsigstore/cosign:v2.2.3 /ko-app/cosign /usr/local/bin/cosign
Copy link
Contributor

@ee7 ee7 Mar 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For visibility: this change is unrelated to this PR, and is because #254 was incomplete.

I will create an issue for this. The checks passed successfully on #254 and the corresponding commit on main (f47aa02) only because the tests workflow was skipped, but it isn't skipped on this PR.

Edit: created #255.

@miki725 miki725 merged commit bafac54 into main Mar 25, 2024
2 checks passed
@miki725 miki725 deleted the nettrino/errornit branch March 25, 2024 13:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ee7 ee7 ee7 approved these changes

@miki725 miki725 miki725 left review comments

@viega viega Awaiting requested review from viega viega is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

surface expected error output messages at trace level or filter them
3 participants
@nettrino @miki725 @ee7